chore: configure ruff lint rules and fix what they surfaced - #883
Draft
maxwbuckley wants to merge 1 commit into
Draft
chore: configure ruff lint rules and fix what they surfaced#883maxwbuckley wants to merge 1 commit into
maxwbuckley wants to merge 1 commit into
Conversation
9 tasks
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Contributor
|
📝 Docs preview is not auto-deployed for fork PRs. A maintainer with write access to |
maxwbuckley
force-pushed
the
chore/ruff-lint-rules
branch
from
August 4, 2026 14:20
093fc74 to
60dd35f
Compare
The ruff and ruff-format pre-commit hooks were running with no configuration
at all, so only ruff's built-in defaults (E4/E7/E9 + F) were ever enforced.
Adds a [tool.ruff] section selecting the correctness-oriented groups:
B, C4, PIE, PERF, PLE, PLW, LOG, G, ASYNC and RUF. Formatting is unchanged
(ruff-format defaults, which the tree already matches).
Everything in `select` is currently clean, so the hook is enforceable as-is.
Each entry in `ignore` is a deliberate call with its reason inline; the
notable ones:
- B905 (zip strict=) is a per-call-site behaviour change, turning a silent
truncation into a runtime exception. Worth adopting deliberately, not in a
lint sweep.
- RUF005, C408 and RUF007 are style-only rewrites of code that is already
correct and readable (`a + [b]` -> `[*a, b]`, dict(a=1) -> {"a": 1},
zip(x[:-1], x[1:]) -> itertools.pairwise(x)). Enforcing them means churning
working call sites for no behaviour change, so they are left to author
preference.
- RUF022 sorts __all__ alphabetically, which scrambles the semantic grouping
comments the schema/package __init__ files rely on.
- RUF100 is evaluated against `select`, so it flags every noqa written for a
rule not yet enabled (BLE001, PLC0415, N803, ...). Re-enable once those
groups are adopted.
No live defects were found. Every rule fired on code that behaves correctly
today; what follows removes fragility, not bugs.
Correct today, fragile to a later change:
- LOG014: _record_error passes exc_info=True, which reads the ambient
sys.exc_info(). Both of its callers invoke it from inside an except block,
so the traceback is logged correctly; the rule is lexical and fires because
the logging call sits in a helper rather than in the handler itself.
Passing the exception explicitly makes it independent of the caller's
context.
- B023: a closure in the frame loop captured the loop variable `phase` by
reference. It is called immediately in the same iteration, so the value was
always correct. `fill` is hoisted above the loop and takes `phase` as a
parameter, which also stops re-creating the function object every frame.
Binding it as a default argument would satisfy the rule equally.
- B011: `assert False` in a test, which python -O strips, turning a failure
into a silent pass. The suite is not run under -O today. Replaced by calling
the constructor directly, so an exception fails the test with its own
traceback.
- B017: a blind pytest.raises(Exception) that would also accept an unrelated
failure. The test passes for the right reason today; naming the accepted
exception set keeps it that way.
- RUF043: pytest match="libcloudxr.so" treats '.' as a regex wildcard where a
literal filename was meant. The real message contains the literal, so the
assertion passes correctly, but it is weaker than it reads. The remaining
patterns are intentional regexes and are now raw strings.
Typing and explicitness:
- RUF012: three mutable class attributes annotated ClassVar, one of them on
the EnvConfig singleton.
- RUF013: implicit Optional spelled out as `str | None`.
- B904: `raise ... from` on re-raises, so the original cause is not lost.
- G004: log calls take %s arguments rather than eagerly formatted f-strings.
- PLW1510: subprocess.run calls that inspect returncode say check=False.
The tree already spelled this out at 10 call sites, 8 of them in
oob_teleop_adb.py; this covers the stragglers.
TRY004 (ValueError -> TypeError in TeleopSessionConfig validation) was left
alone: it is a public API behaviour change, not a lint fix.
Verified on Ubuntu 24.04 / Python 3.12: ruff check and ruff format --check
both clean at v0.15.1, the version pinned in .pre-commit-config.yaml, and
SKIP=check-copyright-year pre-commit run --all-files passes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Max Buckley <maxwbuckley@gmail.com>
maxwbuckley
force-pushed
the
chore/ruff-lint-rules
branch
from
August 4, 2026 14:26
60dd35f to
69137c8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The
ruffandruff-formatpre-commit hooks were running with no configuration atall, so only ruff's built-in defaults (E4/E7/E9 + F) were ever enforced. Adds a
[tool.ruff]section selecting the correctness-oriented groups: B, C4, PIE, PERF,PLE, PLW, LOG, G, ASYNC and RUF. Formatting is unchanged (ruff-format defaults, which
the tree already matches).
Everything in
selectis currently clean, so the hook is enforceable as-is. Each entryin
ignoreis a deliberate call with its reason inline; the notable ones:B905(zipstrict=) is a per-call-site behaviour change, turning a silenttruncation into a runtime exception. Worth adopting deliberately, not in a lint sweep.
RUF005,C408andRUF007are style-only rewrites of code that is already correctand readable (
a + [b]→[*a, b],dict(a=1)→{"a": 1},zip(x[:-1], x[1:])→itertools.pairwise(x)). Enforcing them means churning workingcall sites for no behaviour change, so they are left to author preference.
RUF022sorts__all__alphabetically, which scrambles the semantic groupingcomments the schema/package
__init__files rely on.RUF100is evaluated againstselect, so it flags everynoqawritten for a rule notyet enabled (
BLE001,PLC0415,N803, ...). Re-enable once those groups are adopted.No live defects were found. Every rule fired on code that behaves correctly today;
what follows removes fragility, not bugs.
Correct today, fragile to a later change
LOG014:_record_errorpassesexc_info=True, which reads the ambientsys.exc_info(). Both of its callers invoke it from inside anexceptblock, so thetraceback is logged correctly; the rule is lexical and fires because the logging call
sits in a helper rather than in the handler itself. Passing the exception explicitly
makes it independent of the caller's context.
B023: a closure in the frame loop captured the loop variablephaseby reference.It is called immediately in the same iteration, so the value was always correct.
fillis hoisted above the loop and takesphaseas a parameter, which also stopsre-creating the function object every frame. Binding it as a default argument would
satisfy the rule equally.
B011:assert Falsein a test, whichpython -Ostrips, turning a failure into asilent pass. The suite is not run under
-Otoday. Replaced by calling theconstructor directly, so an exception fails the test with its own traceback.
B017: a blindpytest.raises(Exception)that would also accept an unrelatedfailure. The test passes for the right reason today; naming the accepted exception set
keeps it that way.
RUF043: pytestmatch="libcloudxr.so"treats.as a regex wildcard where a literalfilename was meant. The real message contains the literal, so the assertion passes
correctly, but it is weaker than it reads. The remaining patterns are intentional
regexes and are now raw strings.
Typing and explicitness
RUF012: three mutable class attributes annotatedClassVar, one of them on theEnvConfigsingleton.RUF013: implicit Optional spelled out asstr | None.B904:raise ... fromon re-raises, so the original cause is not lost.G004: log calls take%sarguments rather than eagerly formatted f-strings.PLW1510:subprocess.runcalls that inspectreturncodesaycheck=False. Thetree already spelled this out at 10 call sites, 8 of them in
oob_teleop_adb.py; thiscovers the stragglers.
TRY004(ValueError→TypeErrorinTeleopSessionConfigvalidation) was leftalone: it is a public API behaviour change, not a lint fix.
Python only — no C++ or CMake is touched. A companion PR adds the C++ warning set; the
two are independent and can land in either order.
Type of change
Testing
Ubuntu 24.04 / x86_64, Python 3.12 —
ruff checkandruff format --checkboth clean at v0.15.1, the version pinned in.pre-commit-config.yaml(268 files).SKIP=check-copyright-year pre-commit run --all-files: all hooks pass.ctest: 309/310. The one failure,cloudxr_test_launcher, is the missing CloudXR SDK(no NGC key on this host, so the download 404s and
get_sdk_path()raises) — anenvironment gap, not a code failure.
Forward-compat note, not addressed here: at ruff 0.16
format --checkwants to reflowPython code blocks embedded in three Markdown files (
examples/teleop_ros2/README.md,src/plugins/oak/README.md, and one other) — newer ruff formats fenced code in Markdown,which 0.15.1 does not. None of those files are touched by this PR, but it will surface
whenever the pre-commit rev is bumped past 0.16.
Checklist
SKIP=check-copyright-year pre-commit run --all-filesgit commit -s) per the DCODocumentation: no user-facing behaviour changes, so no doc updates. Every non-obvious
ignoreentry is documented inline inpyproject.toml.Tests: no new tests — this is a lint-configuration change plus the fixes it surfaced,
and it is exercised by the existing suite passing.